🔍 Precision bug-hunting review — 2 defects found

Scoped to concrete defects only (logic/data-loss/error-path). Style, naming, architecture, and test coverage are covered by the general review and intentionally not repeated here.

Note: the inline-comment endpoint returned a transient GitHub 500, so both findings are consolidated here instead of posted as inline review comments.

1. auto_commit swallows commit failures → silent data loss (critical)

codeframe/core/worktrees.py:97-119 runs git add -A and git commit with capture_output=True and no check/return-code inspection. If the commit fails, auto_commit still logs "Auto-committed …" and returns True.

Failure scenario: in any environment without a configured git identity (no user.name/user.email — the default in many CI runners, containers, and fresh installs; existing repo history does not supply it), the agent completes and edits files in the worktree. git add -A stages them; git diff --cached --quiet returns 1, so the code proceeds to git commit, which fails with "Author identity unknown". The nonzero return code is swallowed and auto_commit returns True. merge_back then runs git merge cf/<task> which is "Already up to date" (no new commit on the branch) → success=True, so in runtime.execute_agent worktree_merged=True and the finally block calls cleanup() — git worktree remove --force destroys the worktree (with the still-uncommitted index) and git branch -D deletes the branch. Net result: run marked COMPLETED, base branch unchanged, the agent's file edits permanently destroyed. This is the exact silent-data-loss regression #787 exists to prevent. (A failing pre-commit hook produces the same outcome; an git add -A failure has the analogous "returns False ⇒ nothing to merge ⇒ cleanup destroys the worktree" path.)

Suggested fix (add check=True; same for git add -A):

         subprocess.run(
             ["git", "commit", "-m", f"cf: auto-commit worktree changes for {task_id}"],
             cwd=str(worktree_path),
             capture_output=True,
             text=True,
+            check=True,
         )

With check=True, a commit/add failure raises and propagates to execute_agent's outer except Exception (runtime.py:943), which marks the run FAILED and — because worktree_merged stays False — calls preserve(), keeping the worktree and branch for recovery instead of destroying them.

2. Preserved branch makes worktree.create() crash on retry → stranded run (major)

_create_worktree_context (context.py:155) calls worktree.create(), which does git worktree add -b cf/<task_id> … <base> with check=True. A preserved cf/<task_id> from a prior run (the PR's own preserve() path on failure/conflict) makes this raise on the next attempt.

Failure scenario: Run 1 ends via preserve() (agent failure or merge conflict) → the cf/T branch and .codeframe/worktrees/T are intentionally left on disk. The user retries cf work start T --execute --isolation worktree. create_execution_context (runtime.py:715) runs before the try: at runtime.py:723, so _create_worktree_context → worktree.create() → git worktree add -b cf/T … fails with "fatal: a branch named 'cf/T' already exists" (and/or the worktree dir already exists). The CalledProcessError is not caught by runtime's try/except/finally (no fail_run; finally never runs) nor by the CLI's except ValueError (cli/app.py:2572 — CalledProcessError is not a ValueError), so it surfaces as a raw traceback and run T is left IN_PROGRESS. Every subsequent retry fails identically until cf/T and the worktree dir are deleted by hand. This is a regression specific to this PR's preserve() design — previously WORKTREE was hard-rejected before any branch existed.

The fix is not local to one line: either move create_execution_context inside the try (so a collision becomes a handled failure that fails + preserves, rather than an escaped exception), or have _create_worktree_context detect an existing preserved cf/<task_id> and reuse/reset it explicitly instead of letting git worktree add -b crash.

Non-issues verified (traced against real code, discarded): rebased_workspace is safe — Workspace.state_dir is an independent field and db_path derives from state_dir (not repo_path), so the main DB stays put. The "dirty main tree" case is handled correctly: git checkout base_branch is a no-op (already on it), and a conflicting dirty tree fails at git merge (returncode ≠ 0), which routes through the conflict → blocker → preserve() path as the PR describes.

Review complete — no files modified, no commits pushed.

View job run

Review: #867 — re-enable worktree isolation with real merge-back

Solid design overall — rebased_workspace cleanly separates "code root" from "state root," and the merge/preserve/cleanup wiring in runtime.execute_agent correctly avoids the #714 silent-discard bug for the success/failure/conflict paths that are actually tested (tests/core/test_worktree_isolation.py covers all three well with a real git repo + fake adapter). A few things worth addressing before/soon after merge:

Major — merge-back races on the shared main working tree

TaskWorktree.merge_back() (codeframe/core/worktrees.py:121-188) runs git checkout base_branch then git merge directly in repo_path — the single shared main working directory — with no locking anywhere in this path. Two cf work start --execute --isolation worktree invocations for different tasks run concurrently (e.g. two terminals against the same repo, which is exactly the workflow this feature is meant to make safe) will interleave checkouts/merges in that same directory. That can corrupt the working tree, land a merge on the wrong base commit, or produce spurious conflicts — undermining the isolation this feature is supposed to provide. Worth a lock around merge-back (e.g. a lockfile keyed on repo_path, similar to the existing _registry_lock pattern in the same module) or at least an explicit documented caveat that concurrent single-run worktree executions against the same repo aren't safe yet.

Major — merge conflict downgrades state.status but not agent_status, so metrics/cost stats end up wrong

In codeframe/core/runtime.py (~lines 819-903): agent_status is computed once from result.status before merge-back runs, and is what's later passed to engine_stats.record_run(status=agent_status.value.upper(), ...). When merge-back conflicts, state is correctly reassigned to AgentState(status=AgentStatus.BLOCKED) (and the blocker/run-status transition uses state.status correctly), but agent_status still holds COMPLETED. So a worktree run that conflicts on merge-back gets permanently recorded in engine_stats (and by extension the Costs page, per Phase 5.2) as COMPLETED even though the task is actually BLOCKED. Suggest updating agent_status alongside state.status in the conflict branch (or just deriving the metrics status from state.status at that point).

Moderate — a dirty main tree doesn't actually produce the documented "conflict blocker"

The PR description's "Known limitations" says a dirty main tree "surfaces as a conflict blocker (branch preserved)." But merge_back()'s git checkout base_branch call uses check=True; if the main tree has conflicting local changes, that checkout raises CalledProcessError, which isn't caught by _merge_back() or by the merge-conflict handling in runtime.py (lines ~843-860) — it propagates to the outer except Exception at line ~943, so the run instead fails as a generic FAILED with an "Unhandled error" log rather than a blocker carrying conflict details. No data loss (the finally block still preserves the worktree/branch since worktree_merged stays False), but the user experience doesn't match what's documented. Might be worth catching the checkout failure explicitly and routing it through the same blocker path as a real merge conflict, or correcting the docs.

Minor — stale docstring example

work_batch_run's docstring in cli/app.py still lists codeframe work batch run --all-ready --isolation worktree as an example, even though this exact function now explicitly rejects --isolation worktree with a dedicated error message. This line predates the PR (it was already stale under the old #714 hard-reject), but since this PR touches this function's rejection logic directly, it'd be a good time to fix the example.

Minor — merge-back's checkout can silently switch the main tree's branch

base_branch is captured once at worktree creation via get_base_branch(repo_path). If anything changes the main tree's checked-out branch between worktree creation and merge-back, merge_back() will silently check it back out to base_branch with no restoration afterward. Not destructive on its own (a real conflicting dirty tree fails per the point above), but worth calling out beyond "a dirty tree conflicts" — the main tree shouldn't be touched at all while a worktree run is in flight.

Nits

• No test covers the _create_worktree_context docstring's claim that the worktree is intentionally not registered in WorktreeRegistry (i.e. asserting list_worktrees() stays empty after a worktree run) — worth a small regression test given that's explicitly why orphan cleanup can't nuke a preserved branch.

Test coverage for the three core scenarios (success/conflict/failure) is good. The gaps above (concurrent runs, dirty-main-tree checkout failure, engine-stats mismatch) aren't covered, which is consistent with them slipping through.

Review response — all Critical/Major findings addressed (commit acc06b4)

Thanks to both reviews (opencode/GLM + CI bots) — they converged on the same real defects.

Verified non-issues (agreed): rebased_workspace keeps state on the main DB (db_path derives from the untouched state_dir); no double-apply path.

Demo — #787 acceptance criteria → outcome evidence

This is a CLI/core feature (no web surface); the evidence is the integration suite
exercising real git worktrees + a real Workspace (fake adapter for determinism),
asserting real filesystem/git state — not "exited 0".

Follow-up review — verified the hardening commit (acc06b4)

I re-read the full base→head diff (codeframe/core/worktrees.py, sandbox/context.py, runtime.py, cli/app.py, adapters, and the new/updated tests) against my prior review and the CI bot's review on this PR. All five Critical/Major/Moderate findings are fixed correctly, not just cosmetically:

• auto_commit silent data loss (critical) — git add/git commit now check returncode and raise RuntimeError (worktrees.py:140-167). A raise from _merge_back() is caught in runtime.py:855-874, turned into a blocker, and routes to exec_ctx.preserve() in the finally (runtime.py:1005-1009) instead of cleanup(). Regression test test_commit_failure_raises_not_silent exercises this with a real rejecting pre-commit hook — good, that's a more realistic repro than a missing git identity.
• Preserved-branch collision stranding IN_PROGRESS (major) — create_execution_context moved inside execute_agent's try (runtime.py:719-726), and _create_worktree_context now pre-checks for a leftover cf/<task_id> branch or worktree dir and raises a clear ValueError (context.py, ~30 lines added) instead of letting git worktree add throw a raw CalledProcessError. Since exec_ctx stays None when this raises, the finally correctly does nothing (nothing was created this attempt) and the outer except Exception marks the run FAILED. Covered by test_preserved_branch_collision_raises_clear_error.
• Concurrent merge-back on the shared main tree (major) — _main_tree_lock (worktrees.py:39-59) wraps checkout+merge+abort in a flock on .git/cf-merge.lock. Confirmed the lock is released correctly on both the success and conflict-abort return paths, and that it uses LOCK_EX on a real fd (flock is per-open-file-description, so this actually serializes across threads and processes, not just processes — better than what the docstring undersells).
• agent_status/engine_stats mismatch (major) — agent_status = state.status re-sync now happens right before engine_stats.record_run (runtime.py:896-899), after any merge-back downgrade to BLOCKED. Confirmed engine_stats.py already accepts BLOCKED as a valid status value, so this is a complete fix, not just a type-compatible one.
• Dirty-tree checkout failure → generic FAILED instead of documented blocker (moderate) — now caught by the same try/except Exception around exec_ctx.merge_back(), producing a blocker + preserve. Matches the "Known limitations" wording in the updated PR description.

The one minor item from my first pass — merge_back()'s git checkout base_branch can silently switch the main tree's branch with no restoration — is still open. That's consistent with the author's response (only Critical/Major/Moderate were in scope for this follow-up), and it's genuinely low severity given the dirty-tree case is now handled via the blocker path. Not blocking.

Two small things worth a look (optional, not blocking)

1. No regression test pins the engine_stats fix. test_conflict_creates_blocker_and_preserves_branch in tests/core/test_worktree_isolation.py verifies the run ends BLOCKED and the branch is preserved, but doesn't assert what engine_stats recorded. Since that table is now user-facing (Costs page, Phase 5.2), a one-line query against engine_stats after the conflict test would lock in the exact bug this commit fixes and prevent a silent regression later.
2. Cosmetic: runtime.py:904-907 logs f"Agent blocked: {result.blocker_question or 'Unknown'}" for all BLOCKED outcomes, including the new merge-conflict path — since result.blocker_question is never set there, the log line always reads "Agent blocked: Unknown" even though the real blocker (with the actual conflict details) was created correctly. Doesn't affect behavior, just makes the log line less useful for this new path.

No new correctness, security, or data-loss issues found in the hardening commit itself — subprocess calls throughout use list-form args (no shell=True), so no injection surface. Test coverage for the three core scenarios (success/conflict/failure) plus the two regressions is solid and uses a real git repo rather than mocks, which is the right call for this kind of state-machine-over-git logic.